Skip to content

Conversation

@SriramGuduri
Copy link

Summary

< Provide a brief description of the changes in this PR >

Some conventions to follow

  1. add the module name as a prefix
    • for example: add a prefix: docstore: for document store module, blobstore for Blob Store module
  2. for a test only PR, add test:
  3. for a perf improvement only PR, add perf:
  4. for a refactoring only PR, add "refactor:"

@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Sriram Guduri <s***@s***.i***.s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated.

@SriramGuduri SriramGuduri force-pushed the W-20111589/iamCreateIdentityGCP branch 2 times, most recently from 10d3d06 to 022911e Compare November 12, 2025 10:20
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2025

Codecov Report

❌ Patch coverage is 80.83333% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.20%. Comparing base (ff787e8) to head (1de95fa).

Files with missing lines Patch % Lines
...ava/com/salesforce/multicloudj/iam/gcp/GcpIam.java 81.25% 18 Missing and 3 partials ⚠️
...m/salesforce/multicloudj/iam/client/IamClient.java 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #142      +/-   ##
============================================
- Coverage     83.25%   83.20%   -0.06%     
- Complexity       36       55      +19     
============================================
  Files           148      149       +1     
  Lines          7697     7811     +114     
  Branches        898      906       +8     
============================================
+ Hits           6408     6499      +91     
- Misses          856      876      +20     
- Partials        433      436       +3     
Flag Coverage Δ
unittests 83.20% <80.83%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SriramGuduri SriramGuduri force-pushed the W-20111589/iamCreateIdentityGCP branch from 022911e to fa1d1dc Compare November 12, 2025 10:31
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @SriramGuduri to sign the Salesforce Inc. Contributor License Agreement.

@SriramGuduri SriramGuduri force-pushed the W-20111589/iamCreateIdentityGCP branch from fa1d1dc to 6285df8 Compare November 14, 2025 05:41
import java.util.Optional;
import java.util.stream.Collectors;

@SuppressWarnings("rawtypes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this. Also noticed that in AbstractIAM, we don't need the generic type T

public abstract class AbstractIam<T extends AbstractIam<T>> implements Provider, Identity

It's un-used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this in the following commits.


@SuppressWarnings("rawtypes")
@AutoService(AbstractIam.class)
public class GcpIam extends AbstractIam<GcpIam> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, this will be public class GcpIam extends AbstractIam

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this in the following commits.

public GcpIam(Builder builder) {
super(builder);
try {
this.iamClient = IAMClient.create();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should get the client from the builder, not create it here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Addressed this in the following commits.

try {
policy = iamClient.getIamPolicy(getRequest);
} catch (ApiException e) {
// If policy doesn't exist, create a new one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we have not idea why the exception occurred, maybe it was just 5xx. Let's try to narrow down

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking aloud, since we are creating new identity here, what are the scenarios where policy would and would not exists? should be a single scenario

Copy link
Author

@SriramGuduri SriramGuduri Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we have not idea why the exception occurred, maybe it was just 5xx. Let's try to narrow down

  • The Idea is if getIamPolicy gives a 404 NOT FOUND error, create a new one. But yeah ApiException could be anything from a 5xx to a 4xx. I'll narrow down the check to 404 http status.

thinking aloud, since we are creating new identity here, what are the scenarios where policy would and would not exists? should be a single scenario

  • The scenario I can think of here was if identity already existing with a Policy attached to it. But seems like GCP doesn't restrict Display Names to be unique. This might not be a valid scenario to start with. I'll validate this once I have the GCP access and course correct.

@SriramGuduri SriramGuduri force-pushed the W-20111589/iamCreateIdentityGCP branch from 8f7a02b to b8fcdf3 Compare November 17, 2025 06:51
Comment on lines 44 to 50
public GcpIam(Builder builder, IAMClient iamClient) {
super(builder);
if (iamClient == null) {
throw new InvalidArgumentException("IAMClient cannot be null");
}
this.iamClient = iamClient;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this constructor now since we are building the iamClient in the builder if not present.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll address this in following commits.

/**
* Integration tests for GcpIam.
*
* <p>This test class follows the conformance testing pattern used across the MultiCloudJ project.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gen ai comments be modified or removed, it's implicit that it should follow the project patterns

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad :-P. I'll remove them.

* natively support. Therefore:
* <ul>
* <li>Recording mode (-Drecord) works by connecting directly to real GCP APIs</li>
* <li>Replay mode is not yet implemented and will fail</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should try the rest client until we have the grpc enabled and avoid skipping the tests.
grpc is supported by wiremock here https://github.com/wiremock/wiremock-grpc-extension

Copy link
Author

@SriramGuduri SriramGuduri Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was facing issue with http json transport earlier. I had a sync up with Yamini on this. I'll address this in the following commits.

Comment on lines 76 to 77
// Replay path - inject mock credentials
// Note: Since WireMock doesn't support gRPC, replay mode is not yet implemented.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Author

@SriramGuduri SriramGuduri Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. I'll address this in the following commits.

Comment on lines 109 to 163
/**
* Indicates whether the provider supports creating identities.
*
* @return true if createIdentity is supported
*/
boolean supportsCreateIdentity();

/**
* Indicates whether the provider supports creating identities with trust configuration.
*
* @return true if createIdentity with trust config is supported
*/
boolean supportsCreateIdentityWithTrustConfig();

/**
* Indicates whether the provider supports getting identity details.
*
* @return true if getIdentity is supported
*/
boolean supportsGetIdentity();

/**
* Indicates whether the provider supports deleting identities.
*
* @return true if deleteIdentity is supported
*/
boolean supportsDeleteIdentity();

/**
* Indicates whether the provider supports attaching inline policies.
*
* @return true if attachInlinePolicy is supported
*/
boolean supportsAttachInlinePolicy();

/**
* Indicates whether the provider supports getting inline policy details.
*
* @return true if getInlinePolicyDetails is supported
*/
boolean supportsGetInlinePolicyDetails();

/**
* Indicates whether the provider supports listing attached policies.
*
* @return true if getAttachedPolicies is supported
*/
boolean supportsGetAttachedPolicies();

/**
* Indicates whether the provider supports removing policies.
*
* @return true if removePolicy is supported
*/
boolean supportsRemovePolicy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not add this for each func here, instead the tests can be skipped with Assumptions inside the @test.

Copy link
Author

@SriramGuduri SriramGuduri Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. I'll address this in the following commits.

Comment on lines 257 to 260
public Builder withCredentials(GoogleCredentials credentials) {
this.credentials = credentials;
return self();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how will this be used ? the user never supply the GoogleCredentials directly

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to one of the services that supplied credentials to the builder. I'll clean this up.

*
* @return list of WireMock extension class names
*/
List<String> getWiremockExtensions();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add when we need it. not only interfaces need to define this extensions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@SriramGuduri SriramGuduri force-pushed the W-20111589/iamCreateIdentityGCP branch from b8fcdf3 to 1de95fa Compare November 20, 2025 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants